Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 1-Implement-and-rewrite-test#1259
Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 1-Implement-and-rewrite-test#1259Jacknguyen4438 wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
…e first course work test
cjyuan
left a comment
There was a problem hiding this comment.
- Function implementation is correct.
Describing tests can be quite challenging. Feel free learn from AI how to keep the test descriptions concise.
| !cardRanks.includes(card.slice(0,-1)) || | ||
| !cardSuits.includes(card.slice(-1)) | ||
| ) { | ||
| throw new Error("Invalid card"); | ||
| } | ||
|
|
||
| switch (card.slice(0, -1)) { |
There was a problem hiding this comment.
Could consider storing the extracted rank and suit character in some variables first.
!cardRanks.includes(rank) || !cardSuits.includes(suit)
is probably more readable than
!cardRanks.includes(card.slice(0,-1)) || !cardSuits.includes(card.slice(-1))
There was a problem hiding this comment.
Thank you for the feedback I understand the issue here I will replace the slice and try your suggestion to make the code cleaner. Here is my solution:
let rank = card.slice(0,-1);
let suit = card.slice(-1);
if (
typeof card !== "string" ||
!cardRanks.includes(rank) ||
!cardSuits.includes(suit)
) {
throw new Error("Invalid card");
}
| test(`should return "Invalid angle" when ( 180 < angle < 360)`, () => { | ||
| expect(getAngleType(-1)).toEqual("Invalid angle"); | ||
| expect(getAngleType(-10)).toEqual("Invalid angle"); | ||
| expect(getAngleType(361)).toEqual("Invalid angle"); |
There was a problem hiding this comment.
-
The test description is not quite correct.
-
Why not test both boundary cases?
There was a problem hiding this comment.
Thank you for the feedback I see that I haven't write a test case that check if what happen when the argument is equal to 180 or 360 here is my solution:
test(should return "Invalid angle" when ( 0 < angle && angle > 360), () => {
expect(getAngleType(-1)).toEqual("Invalid angle");
expect(getAngleType(-10)).toEqual("Invalid angle");
expect(getAngleType(361)).toEqual("Invalid angle");
expect(getAngleType(0)).toEqual("Invalid angle");
expect(getAngleType(360)).toEqual("Invalid angle");
});
| test(`should return false when denominator is infinity`, () => { | ||
| expect(isProperFraction(1, 23443243n)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
-
The test description does not quite match the test.
-
A large "bigint" type value is not the same as infinity. The closest thing to infinity in JavaScript is the predefined named constant
Infinity-- it is a value of type "number".
There was a problem hiding this comment.
Thank you for the feedback, I see that I have mistake a bigINt number type to infinity here is my solution:
test(should return false when denominator is bigInt, () => {
expect(isProperFraction(1, 23443243n)).toEqual(true);
});
| test(`should return false when numerator is negative`, () => { | ||
| expect(isProperFraction(-1, 2)).toEqual(false); | ||
| }); | ||
| test(`should return false when denominator is negative`, () => { | ||
| expect(isProperFraction(1, -2)).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
These test descriptions are not quite accurate.
For proper fractions, the condition is, "the absolute value of denominator is larger than the absolute value of the numerator". So we can create a category for proper fractions and describe it as:
should return true when abs(numerator) < abs(denominator).
There was a problem hiding this comment.
Thank you for the feed back, I have make change to this part so when you check again it should be correct order
| test("Should return 2 for '2♠'", () => { | ||
| expect(getCardValue("2♠")).toEqual(2); | ||
| }); | ||
|
|
||
| test("Should return 5 for '5♥'", () => { | ||
| expect(getCardValue("5♥")).toEqual(5); | ||
| }); | ||
|
|
||
| test("Should return 9 for '9♦'", () => { | ||
| expect(getCardValue("9♦")).toEqual(9); | ||
| }); | ||
|
|
||
| test("Should return 10 for '10♣'", () => { | ||
| expect(getCardValue("10♣")).toEqual(10); | ||
| }); |
There was a problem hiding this comment.
When preparing tests, we should ensure the tests cover all possible cases. If we specify a test for individual card, we will need about 53 tests to cover all possible cases. Instead, we could consider classifying all possible values into different categories, and then within each category we test some samples.
For example, one possible category for getCardValue() is, "should return the value of number cards (2-10)", and we can prepare the test as
test("should return the value of number cards (2-10)", () => {
expect(getCardValue("2♣︎")).toEqual(2);
expect(getCardValue("5♠")).toEqual(5);
expect(getCardValue("10♥")).toEqual(10);
// Note: We could also use a loop to check all values from 2 to 10.
});
Can you practice preparing tests in this fashion?
There was a problem hiding this comment.
Thank you for the feed back, I dont know that I can refactor these test and make it compact as you suggest. I will begin to re organize these test into a related category to make is easier to read.
|
@cjyuan Thank you for the review I have making change base on your PR review. It have been finish and ready to be reviewed again. |
Learners, PR Template
Self checklist
Changelist
I have don't basic understand on how to correctly writing normal test and jest test frame work, I am waiting for this work to be review.
Questions
I have no question.